Skip to content

Conversation

@liggitt
Copy link
Contributor

@liggitt liggitt commented Jul 19, 2017

Fixes #15337
Fixes #15336

The SDN controller was registering shared informer event handlers in a goroutine, so registration raced with informer start. If the registration lost, then SDN event handlers would never get namespace events.

@liggitt
Copy link
Contributor Author

liggitt commented Jul 19, 2017

@deads2k @dcbw PTAL

@deads2k
Copy link
Contributor

deads2k commented Jul 19, 2017

lgetm

too much work to rewire the underlying controller to be more "normal"?

@liggitt
Copy link
Contributor Author

liggitt commented Jul 19, 2017

needs picking back to 3.6 (exists there too), so keeping it minimal for the moment

@deads2k
Copy link
Contributor

deads2k commented Jul 19, 2017

needs picking back to 3.6 (exists there too), so keeping it minimal for the moment

ok

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 19, 2017

continuous-integration/openshift-jenkins/merge Waiting: You are in the build queue at position: 16

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 254aa79

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@dcbw
Copy link
Contributor

dcbw commented Jul 19, 2017

@liggitt it's not immediately clear to me why the fix in ee76e3e is no longer valid? #14633 moved to the goroutine, now this reverts that. Is that OK?

Ah, nevermind, looks like #14633 moved from not-goroutine to a goroutine for all the controllers it touched, but now that's no longer necessary?

@liggitt
Copy link
Contributor Author

liggitt commented Jul 20, 2017

[test]

@liggitt
Copy link
Contributor Author

liggitt commented Jul 20, 2017

#14633 puts all controller initialization in a goroutine isolated from the api startup. it expects each controller's init function to register shared informer event handlers synchronously, spawn workers in goroutines, then return. once all init functions return, the controller code starts the shared informers.

@liggitt
Copy link
Contributor Author

liggitt commented Jul 20, 2017

removed merge tag, updated with potential fix restoring controller role reconciliation at startup. PTAL

@liggitt liggitt added this to the 3.7.0 milestone Jul 20, 2017
@openshift openshift deleted a comment from openshift-bot Jul 20, 2017
@openshift openshift deleted a comment from openshift-bot Jul 20, 2017
@liggitt
Copy link
Contributor Author

liggitt commented Jul 20, 2017

[test][merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d6c198a

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3348/) (Base Commit: 7a48deb) (PR Branch Commit: d6c198a)

@liggitt
Copy link
Contributor Author

liggitt commented Jul 20, 2017

green tests on latest master. merging to unblock the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants